Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix longitude latitude confusion in extract_trajectory preprocessor #1174

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

ledm
Copy link
Contributor

@ledm ledm commented Jun 11, 2021

A minor edit to extract_trajectory, where the latitude and longitude interface was backwards.

Description

Partially addresses #1137.

Link to documentation:


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

A minor edit to bextract_trajectory, where the latitude and longitude interface was backwards.
Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! isn't there a test as well that will need to be changed? or you used identical lat and lon points in the test 😁

@ledm
Copy link
Contributor Author

ledm commented Jun 11, 2021

The test is:

    def test_extract_trajectory(self):
        """Test to extract a trajectory from a (3, 2, 2) cube."""
        result = extract_trajectory(self.grid_3d, [1.5, 2.5], [2., 2.], 2)
        expected = np.ones((3, 2))
        self.assert_array_equal(result.data, expected)

So it shouldn't fail as it's only effectively comparing the shape of the outputs. Whether that is a good enough unit test should like be another issue/PR!

@valeriupredoi
Copy link
Contributor

OK, it's a decent test, given that the numerical data parameters are tested for (cows vs bulls or bulls vs cows is a different matter, the test is happy as long as they're of the same shape then). I'm merging this if you don't need to get more stuff done to it 👍

@valeriupredoi valeriupredoi merged commit 00d838b into main Jun 11, 2021
@valeriupredoi valeriupredoi deleted the typo-fix-in-extract_trajectory branch June 11, 2021 16:14
@ledm
Copy link
Contributor Author

ledm commented Jun 14, 2021

Thanks V. Randomly, while I uncovered this bug in my local version of the code, I did all the editing and merging for this PR in github without ever having a local copy of this branch. It is a great method to quickly correct bugs.

@zklaus zklaus added the bug Something isn't working label Jun 14, 2021
@valeriupredoi
Copy link
Contributor

valeriupredoi commented Jun 14, 2021

yes but be careful since some tests don't run on the pure github edited branch and only get run when merged onto main so it's good to do the bugfixes locally so you can run all the tests and see if there are issues before merging into main

@bouweandela
Copy link
Member

@ledm @valeriupredoi I'm just reviewing our changelog for v2.3 and I'm a bit disappointed by what you came up with as a title for this change. From our contribution guidelines:

The titles and labels of pull requests are used to compile the Changelog, therefore it is important that they are easy to understand for people who are not familiar with the code or people in the project. Descriptive pull request titles also makes it easier to find back what was changed when, which is useful in case a bug was introduced.

Could you please put in a bit more effort next time? 🙏

@ledm
Copy link
Contributor Author

ledm commented Jun 15, 2021

Sorry, that was the title github generated. I'll change it now, post hoc.

@ledm ledm changed the title Update _volume.py Fix longitude latitude confusion in extract_trajectory preprocessor Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants